feat: Allow specifying additional TGW routes in attached VPCs #132
Conversation
|
NOTE: this change has the same bootstrapping problem as #111, but it works correctly when the VPC resources are configured ahead of time (or in a separate state file as we do in our terraform) |
|
Hi @antonbabenko could you please take a look |
|
Good morning. |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi @antonbabenko could you please take a look |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
pinging this again. @antonbabenko were you able to look at this? Or @elkh510 is there someone else that can? |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
Hello, Do you know if there's any progress on the review? Waiting for this implementation. |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
|
Hi Please verify and merge please |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
partcyborg
force-pushed
the
additional-vpc-cidr-blocks
branch
from
d4618e1 to
466ee64
Compare
Hey @pongsakfreshket i need an approval from someone authorized on this project. Unfortunately it looks like your approval doesn't count here as I still can't merge. |
|
Hi |
|
This PR has been automatically marked as stale because it has been open 30 days |
|
Hi |
Adds parameter `tgw_additional_vpc_cidrs` to the `vpc_attachments` map that enables adding additional `aws_route` resources that send traffic across the TGW peering connection. Changes the name of the existing `aws_route` resource from `this` since there are now more than one in the state file.
partcyborg
force-pushed
the
additional-vpc-cidr-blocks
branch
from
466ee64 to
6d7820c
Compare
|
Hello @antonbabenko I have re-based this PR to match the recent changes |
There was a problem hiding this comment.
@antonbabenko I think this is very important change.
@partcyborg please check my comments.
| transit_gateway_attachment_id = tobool(try(local.vpc_attachments_with_routes[count.index][1].blackhole, false)) == false ? aws_ec2_transit_gateway_vpc_attachment.this[local.vpc_attachments_with_routes[count.index][0].key].id : null | ||
| } | ||
|
|
||
| resource "aws_route" "this" { |
There was a problem hiding this comment.
There is no need to introduce additional resources resource "aws_route" "additional_cidrs" and resource "aws_route" "destination_cidr"
Let's simplify the code a bit by looping over the merged map of local.vpc_route_table_destination_cidr and local.vpc_route_table_additional_cidrs like the following:
resource "aws_route" "this" {
for_each = { for x in merge(local.vpc_route_table_destination_cidr,local.vpc_route_table_additional_cidrs) : ${x.rtb_id}_${x.cidr} => {
cidr = x.cidr,
tgw_id = x.tgw_id
rtb_id = x.rtb_id
} }
region = var.region
route_table_id = each.value["rtb_id"]
destination_cidr_block = try(each.value.ipv6_support, false) ? null : each.value["cidr"]
destination_ipv6_cidr_block = try(each.value.ipv6_support, false) ? each.value["cidr"] : null
transit_gateway_id = each.value["tgw_id"]
depends_on = [aws_ec2_transit_gateway_vpc_attachment.this]
}
There was a problem hiding this comment.
@yyarmoshyk this change isn't possible without causing the deletion of all existing aws_route.this resources, as you can't do dynamic moved blocks.
Given that the key for the default route will depend on the current cidr block, there is no way to write a moved block that will move the existing route entry into a key for the current aws_route.this resource.
As I stated in the PR description, this leaves us only two options:
- Keep the
aws_route.thisresource as is, but add theaws_route.additional_cidrsresource which has a map with key. This violates terraform style, but if we want to drop themovedblock I would be okay with that if the module owners are okay with ignoring terraform style. - Keep the change as written which allows us to not destroy the existing route resource but move it to the new entry, while also keeping the
additional_cidrsobject which is a map.
If we just do what you suggest, I would have to remove the moved block which would mean that when users upgrade the terraform module the route entry will be deleted and re-created which could cause traffic disruption. This is highly undesirable in my opinion
| depends_on = [aws_ec2_transit_gateway_vpc_attachment.this] | ||
| } | ||
|
|
||
| moved { |
There was a problem hiding this comment.
with the change above this becomes unneeded.
There was a problem hiding this comment.
As stated above, your suggestion will cause an outage for all users of this module. I really don't think this is the right approach here.
7 participants
Description
Adds parameter
tgw_additional_vpc_cidrsto thevpc_attachmentsmap that enables adding additionalaws_routeresources that send traffic across the TGW VPC attachment.Changes the name of the existing
aws_routeresource fromthissince there are now more than one in the state file.Motivation and Context
In our environment we have two CIDR blocks that need to transit from the VPC across the TGW network:
10.0.0.0/8and172.16.0.0/12. Unfortunately this is not possible to do with the module as it is today.With this change we can add
aws_routeentries for both CIDR blocks to our VPC route tables.Breaking Changes
In order to implement this I had to add a second
aws_routeresource because the existing one is keyed only on route table ID which is no longer a unique value for route resources.Commonly accepted Terraform style rules indicate that
thisis only an acceptable resource name if it is the only one of its type in a module. Since there are now twoaws_routeresources, I changed the name of the existingthisresource todestination_cidrso that this style rule is still valid.While I personally feel that the this is the correct approach, if renaming the resource is undesirable I would fine with reverting the resource rename.
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request